Minor optimization in shiftOut function #82
Conversation
There was a problem hiding this comment.
This change looks good to me. AVR is notoriously bad in bitshifting (there is only a shift-by-one instruction), so doing it incrementally like this is a good way to help the compiler :-)
As for the !! those are two boolean-negation operators. By applying it twice, you effectively make a "convert to boolean" operator (e.g. 0 becomes false, non-0 becomes true). In this case, this helps to make sure either LOW/0/false or HIGH/1/true is passed to digitalWrite.
With your change, in the MSBFIRST case, you're passing 0 or 128 to digitalWrite. In practice, this will work, since digitalWrite for AVR treats all non-zero values as HIGH. Technically, the API (as documented on arduino.cc) only specifies LOW/HIGH (0/1) as valid values, so this violates the API. You could try adding it again (e.g. digitalWrite(dataPin, !!(val & 1))) but that probably adds a few instructions again :-)
I'm not really sure if the !! is really needed here. Even when not documented explicitly, the API of digitalWrite can be assumed to be 0/non-0 (especially since this call is inside the AVR core itself, so portability to other cores is not really an issue right now).
Summarizing: Looks good to merge as-is.
|
Hi @matthijskooijman thank you for your review! |
|
Good clean up. But it feels slightly wrong to depend on an undocumented API behavior, to allow |
|
You're right, I'm gonna fix it and commit it again! Thank you for your fast reply! |
|
Ok I fixed that, now digitalWrite will receive 1 or 0 both for LSBFIRST and MSBFIRST! |
There was a problem hiding this comment.
@luco5826 did you check if the change added more instructions?
Looks good like this. It would be good to merge both commits, but I guess that can be done while merging.
|
Hi @matthijskooijman ! Right now I checked for the compiler result, there still is 13-15 lines of difference (under plain GCC). |
|
Hi everyone! |
|
Hey @luco5826, thanks for testing. However, I'm a bit confused. AFAICS there's three versions involved: The original, the optimized version without You talk about the number of lines, is this the number of lines in the dissassembly output? Note that some instructions are 2 bytes and some are 4, so it might be better to compare the compiled code size instead (or, to do a local comparison, count bytes inside the |
|
Hi @matthijskooijman ! I don't know any other ways to test the differences, I tried to compile the code with |
Hi!
I found this -very-little optimization in a function I was using with a 595 shift registers' project: while I was trying to understand what this function used to do, I figured out a (IMHO) cleaner solution.
Compiling an example with
gcc -Sgenerated a file 10-15 instruction lines shorter than the previous version, which could lead (i think) to a smaller workload for such a simple operation.I didn't quite understand the double exclamation marks before the shift operation
!!(val & ...so I don't know if this breaks something. Let me know!Keep up with Arduino 'cause it's awesome!